Feat/ecsm update#753
Conversation
|
/bench |
Benchmark — ethrex 20 transfers (median of 3)Table parallelism: auto (cores / 3)
Commit: 1cb8988 · Baseline: cached · Runner: self-hosted bench |
|
/ai-review |
Codex Code ReviewFindings
Open Question
No higher-severity safety, VM semantics, or performance issues found in the PR diff. I did not build or test, per the review constraints. |
Review — Feat/ecsm updateReviewed the EC_SCALAR→ECSM inlining, the xG<p range check, the disjoint memory timestamps, the The substantive changes look correct and sound:
Minor — stale doc comments not updated by this PR:
No blocking issues. |
AI ReviewPR #753 · 12 changed files Findings
Status column reflects the verdict from the verifier: deepseek-verifier (openrouter/deepseek/deepseek-v4-pro). AI-001: Q1[32] range constraint weakened from IS_BIT to AreBytes
Claim The old code constrained q1[32] via an unconditional IS_BIT transition constraint (forcing 0 or 1), but the new code uses AreBytes (allowing 0..255). AreBytes[32] is a stricter bound than IS_BIT, but the corresponding IS_BIT transition constraint is no longer created. Evidence Old code: Suggested fix No functional change needed; both forms soundly constrain q1[32]. If the design intent is to constrain q1[32] to be a bit (e.g. because it carries the high bit of a shifted quotient), add an explicit IS_BIT constraint. Otherwise, document the change. AI-002: len_k column is a free variable constrained only by the BIT bus
Claim The LEN_K column has no direct AIR transition constraint enforcing that its value matches the MSB position of the scalar k. Soundness relies entirely on the BIT bus balancing: if LEN_K doesn't equal msb_position(k), the bus is imbalanced and the verifier detects it. Evidence The ECSM Bit sender is Suggested fix Optionally add a transition constraint that ties LEN_K to k_bit (e.g., LEN_K = argmax of k_bit positions). Alternatively, document that LEN_K is implicitly constrained by the BIT bus balance. AI-003: 256 IS_BIT constraints on k_bit columns add significant cost
Claim 256 IS_BIT transition constraints on k_bit columns push the ECSM constraint count to 411, more than doubling from 148. The MEMW bus read of k (via k_dword_busvalues) already implicitly constrains k_bit values to be 0 or 1 via the byte reconstruction matching memory, making most of these IS_BIT constraints arguably redundant. Evidence The diff comment on the create_constraints function says "411 total: 1 mu + 256 k bits + 8 xG<p + 146 others." 256 IS_BITs on k_bit is a large share. The byte value Suggested fix Consider whether the IS_BIT constraints are needed for soundness or only for defense-in-depth. If the latter, consider whether the proving-time cost justifies them or whether the BIT bus and MEMW bus interactions are sufficient. AI-004: Test coverage gap for new XgLtP overflow constraint
Claim The new OverflowKind::XgLtP (xG<p) constraints are not exercised by constraints_hold_on_generated_trace, which only iterates over [OverflowKind::KLtN, OverflowKind::XrLtP]. Evidence Looking at prover/src/tests/ecsm_tests.rs lines 97-118, the test only iterates Suggested fix Extend the iteration to include OverflowKind::XgLtP in the test, mirroring the existing KLtN/XrLtP checks. Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
|
Benchmark Results for unmodified programs 🚀
|
Responses to AI reviewersCouncil of AIs
Codex
Claude
|
Motivation
The spec received several updates after PR #657 merged. This PR brings the implementation in sync.
Description
EC_SCALAR inlined into ECSM
The separate EC_SCALAR chip is removed. The scalar
kis now stored as 256 individual bit columns inside ECSM instead of 32 bytes. ECSM directly holds and validates all scalar bits via IS_BIT constraints, and interacts with the BIT bus as a receiver (one per bit). The SERVE_K self-referential bus is eliminated.xG range check in the ECSM AIR
Adds a new column
xG_sub_p(U256HL) and provesxG < pvia the same carry-overflow technique already used fork < NandxR < p. Previously this was only enforced in the executor.Disjoint memory timestamps
Separates the xG and k memory reads into different timestamp slots: xG reads at T, k register read and k dword reads at T+1, xR write at T+2.
q1[32] IS_BYTE
The yG quotient's high byte (
q1[32]) was previously constrained by IS_BIT (polynomial). It is now IS_BYTE (bus lookup), fixing a completeness issue for witnesses whereq1[32] > 1.ECDAS padding simplified
Padding rows now use
q0 = q1 = q2 = 0andop = 1. TheR·Pterm in the λ and xR convolution relations is gated withμ, so it vanishes on padding rows and the relations hold at zero carries without needing to pad quotients tor = 3p.Curve identifier in ECDAS bus
A curve identifier
id(Bit, 0 = secp256k1) is prepended to the ECDAS bus tuple in all interactions, enabling future domain separation between curves.